-
Notifications
You must be signed in to change notification settings - Fork 12
Added functions for ftp support #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
kebab01
commented
Aug 24, 2023
- Added 4 new functions for FTP support
- Added a function to get the remaining space on the FS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Thomas (@kebab01 ),
Excellent Pull Request - thank you!
Please consider making a few changes as per the comments.
Best wishes,
Paul
return SARA_R5_ERROR_OUT_OF_MEMORY; | ||
} | ||
|
||
sprintf(command, "%s=1", SARA_R5_FILE_SYSTEM_LIST_FILES); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment here that AT+ULSTFILE=1 returns the Remaining free FS space expressed in bytes.
(I was worried it was doing a full LS - and would overflow the minimumResponseAllocation)
free(response); | ||
return err; | ||
} | ||
char *responseStart = strstr(response, "+ULSTFILE:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
David's changes - which I like - move us away from hard-coded strings in the code. Is there an elegant way to append the ":" to SARA_R5_FILE_SYSTEM_LIST_FILES ? I.e. avoid hard-coding here; also avoid 'cheating' by defining SARA_R5_FILE_SYSTEM_LIST_FILES_RESPONSE (which includes the colon). Maybe create a new helper function which will append the colon to whatever command is passed?
} | ||
|
||
size_t availableSpace; | ||
responseStart += strlen("+ULSTFILE:"); // Move searchPtr to first char |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
@@ -5648,6 +5718,69 @@ SARA_R5_error_t SARA_R5::getFileBlock(const String& filename, char* buffer, size | |||
|
|||
return SARA_R5_ERROR_SUCCESS; | |||
} | |||
SARA_R5_error_t SARA_R5::getAvailableSize(size_t * size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming this to getAvailableFreeSpace
return err; | ||
} | ||
|
||
SARA_R5_error_t SARA_R5::ftpList(const String &dirName, char * response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming this to ftpListFiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm. Thinking about this more...
The response could be quite large, depending on how many files are in the system. You could easily overrun response if you're not careful...
(I know I cheated with this overload of getFileContents because there you can know in advance how much data is coming.)
So, here, it would be best to use a String - which can grow as required - or pass in the size of response so you can stop writing before you overrun. String gets my vote - same as this overload of getFileContents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where possible I avoided the use of String in my changes to avoid heap problems, even though we're using an ESP32 so heap problems go away on deep sleep :)
I had originally changed a lot of existing methods to use char buffers too, but decided that was too much of an ask for a pull request so reverted them.
I'd be wary of using so many Strings on our Cortex M0+ based nodes which don't reboot after waking up.
You haven't found any problems with long running sketches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David (@dajtxx ),
We haven't noticed any problems but: we also tend to run this on ESP32; and we generally don't put the processor to sleep. In this case you're welcome to twist my arm and go with a char buffer, with size.
Cheers,
Paul
@@ -943,6 +943,10 @@ class SARA_R5 : public Print | |||
SARA_R5_error_t connectFTP(void); | |||
SARA_R5_error_t disconnectFTP(void); | |||
SARA_R5_error_t ftpGetFile(const String& filename); | |||
SARA_R5_error_t ftpChangeWorkingDirectory(const String& dirName); | |||
SARA_R5_error_t ftpCreateDirectory(const String& dirName); | |||
SARA_R5_error_t ftpList(const String& dirName, char * response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
@@ -1011,6 +1015,8 @@ class SARA_R5 : public Print | |||
SARA_R5_error_t getFileContents(String filename, char *contents); // OK for binary files. Make sure contents can hold the entire file. Get the size first with getFileSize. | |||
SARA_R5_error_t getFileBlock(const String& filename, char* buffer, size_t offset, size_t length, size_t& bytes_read); // OK for binary files. Make sure buffer can hold the requested block size. | |||
|
|||
SARA_R5_error_t getAvailableSize(size_t * size); // Get available FS space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto